Reduce browser fixture histories and keep last message rows reachable - #83
Conversation
comp615
changed the title
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear
Reviewed head 50f49ade6c3ee3bfa50452bd551e943327d614ab against exact base 3e1fc7d16d6d4dd4035ed0717d9a5706fc471ad7 (stacked on #81). No actionable code/product/security blockers found. This is a comment, not approval.
The compatibility contract is preserved: smaller fixtures must still exercise their actual paging/geometry/failure boundaries with isolated test state, and channel rows must remain reachable without weakening full-visibility or focused, settled 750ms read dwell.
- Fixture coverage: the resized near-fit/overflow/reading setups retain their substantive assertions, old exact replies still predate the loaded channel head and require a second thread page, and the image journey still requires actual remount/reloads with a verified failing-image placeholder. Shared mutable state, ephemeral keys and browser contexts remain test-owned; deep-history consumers retain explicit sizes.
- Production behavior: the channel-only bottom clearance survives the narrow breakpoint. Read-observation comparisons/dwell, navigation ownership, persisted reading intent and thread scrolling are unchanged. The existing real-browser notification journey now combines overflowing history/read acknowledgment with strict fractional-row reachability at desktop and narrow widths.
- Validation: all 12 hosted checks attached to this head passed. Baseline run 35117382002, attempt 3 and candidate run 35151309592 retain exactly the same 394 functional engine/file/title identities (197 per engine), all passing with zero skips/retries, plus the same five measurement cases. The checked-out synthetic merges are
3fbf98f0d70e59e398cebd36f8e371068136808band22b639f353fc6674f26f06c1b1181cdd2b587e55; their GitHub trees equal the pinned base and head trees respectively. All 293 fixture evidence records per run agree on those clean snapshots.
Hosted timing comparison (observed pair, not a causal or repeatability guarantee):
| Metric | Baseline | Candidate |
|---|---|---|
| Earliest required job start → required-gate completion | 544s | 509s |
| Summed functional test execution | 2,654,452ms | 2,393,322ms |
| Summed measurement execution | 107,363ms | 114,279ms |
| All five browser lanes, summed test execution | 2,761,815ms | 2,507,601ms |
The slowest functional file changes from WebKit initial-position.spec.mjs (107,151ms) to WebKit message-navigation.spec.mjs (97,253ms). Measurement scroll.spec.mjs increases from 98,968ms to 106,300ms; that increase is retained in the totals, not hidden by the functional improvement. Functional WebKit signing totals are 209,693ms → 139,741ms across 144 records each, not all-engine signing totals. Baseline artifacts came from the run-level endpoint because the attempt-specific endpoint was unavailable; artifact timestamps and embedded snapshots agree with attempt 3.
Non-blocking documentation follow-up: docs/browser-testing.md:70-74 still says a legacy large default remains; update it to describe the new small default and explicit scale/geometry opt-ins.
Source/metadata review on the authorized laptop only. No PR checkout, build, test/code execution, CI rerun or live/native GUI acceptance was performed. Contributor-reported local negative controls were inspected as claims, not independently rerun. Existing local-only WebKit measurement exclusions are unchanged; this review does not certify a release or stable CI speedup. Independent fixture and hosted-evidence lanes returned; their findings were integrated and cross-checked against source, logs, JSON reports and commit trees.
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
50f49ad to
242159b
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear
Re-reviewed head 242159b439b81c22fc1ceae4f86f6e013ac083de against exact base 6201ab6aa5e916b95ca22f48ab1457e8812e2792. No actionable code, product, or security blocker found. This is a comment, not approval or merge authorization.
The contract remains: smaller, isolated browser fixtures must preserve their actual paging, geometry, and failure boundaries; channel rows must remain fully reachable without weakening focused, settled, full-visibility 750ms read dwell.
- Rebase assessment: the complete old and current PR patches share stable patch ID
09fc95004798136e16c829375ae2a51379fb89e5. I reviewed all nine changed files and the relevant new-base interactions, rather than treating patch identity as proof of compatibility. Explicit scale cases remain separate from compact geometry cases; the 103-row navigation fixture still puts the old reply outside the initial channel window and requires a second thread page. Image coverage still requires >5,000px traversal, actual remount/reloads, and the failed-image placeholder. - Production boundary: only
Messages.module.csschanges in production. Bottom clearance survives the narrow breakpoint; the fractional-tail assertion requires real overflow, real end navigation, and full visibility at 1440px and 640px. Reading eligibility is unchanged by this PR. The added fractional reflow checks geometry after ordinary dwell; it is not a new independent proof of post-reflow read publication. Thread scrolling and unrelated base features are not expanded into new repair scope. - Validation: at 16:13 UTC, all 12 existing exact-head check runs succeeded, including all four Chromium/WebKit journey shards and
CI required(hosted run). This is existing hosted evidence, not a local execution claim. GitHub still reportedmergeable_state: blocked; I have not certified branch-protection readiness. The previous review retains the old-head case-identity and timing comparison, including the measurement slowdown. Those numbers are not current-head measurements or a repeatable speedup claim.
The independent challenge raised sibling-thread parity and whether the general shell tests should retain larger histories. Neither establishes a new blocker: thread geometry is outside this channel-feed repair and was not reproduced; dedicated readingTest cases still exercise the shared link helper, overflowing panel resize, bottom follow, and reading-anchor restoration (layout.spec.mjs:290-401,565-640). Minimal defaults for non-scroll shell checks are intentional. I am not expanding the previously reviewed contract on those grounds.
The previous non-blocking documentation note remains: docs/browser-testing.md:84 still describes a legacy large default, whereas tests/browser/fixture.mjs:41 now defaults to 1/1. No new impact warrants promoting that note to a blocker.
Source-only review on the pinned Blox host. No PR checkout, dependency installation, build, test, import/source/PR-code execution, CI rerun, or live/native GUI acceptance. Existing CI was observed without waiting or monitoring. Independent challenge and CI lanes were integrated before publication.
Since the reading fixture shrank to 20 tall messages (#83), the row above the anchored one can be taller than a single 300px wheel step, so the first wholly visible paragraph stayed the same message and "panel restoration yields to a new wheel reading position" failed on main in both engines. Keep making bounded, verified wheel progress until the visible reading row belongs to another message, reusing the shared anchor reader. Signed-off-by: Thomas Petersen <thomasp@squareup.com> (cherry picked from commit 664b27e)
2 participants
🤖 AI-authored implementation and description.
Stacked on #81; independent of the semantic migration batch. Small histories are now the default. Geometry and pagination tests opt into enough rows to exercise their actual boundary rather than signing hundreds of unused events in both communities.
All 35 fixture-sizing browser cases remain (70 engine executions). Initial-position generates exact near-fit counts rather than discarding generated rows. Reading/resize cases retain their measured distance outside prefetch; exact navigation retains a reply older than the initial channel head and a second thread page. Image history still forces virtualization/remounts and includes a verified failing image with its placeholder intact. No isolation, broker, timing-budget or retry changes.
Coverage controls: one-row layout failed real scrolling; a 40-row image history failed the remount check. The selected sizes pass. Review caught an obsolete failing-image URL after resizing; the new assertion failed with a successful image and passes with the restored 404 path. All four complete affected files pass in Chromium and WebKit (70/70). Diff and Biome checks pass.
Also retains the fractional-row clipping fix from closed #67, without its server-reuse changes. Bottom clearance keeps the last channel row reachable despite integer scroll extents; narrow layouts preserve that clearance. Strict full-visibility and dwell requirements for marking messages read are unchanged. The existing notification journey adds overflowing history only for the mention case, then checks fractional reflow at 1440px and 640px. No browser cases added or removed: this is real layout/read-acknowledgment coverage, not a DOM-emulator assertion. The regression failed with the mention remaining unread in both Chromium and WebKit without the fix, and passed with it. Rendered desktop and narrow states were inspected in both engines.
For the clipping update, the complete notification, initial-position, layout, completion-layout, image-scroll and message-navigation files passed locally in both engines (88/88), along with all nine read-observation unit tests and
bin/pnpm check. Fresh hosted CI and DCO pass at that head, including all 394 identical browser-journey identities with no skips, failures or retries.The first four timing samples compare the same #81 baseline with the fixture-sizing snapshot, before the clipping fix. The final row measures the updated head. Runner/worker configuration is unchanged. Full gate means earliest job start through required-gate completion, excluding queue time:
Both candidate gates passed, including all 394 identical browser case identities, with no skips, failures or retries. Shared-fixture base history rows fell from 59,880 to 36,878 per engine (38.4% fewer). Summed WebKit history-signing time was 209.7s on the fresh baseline versus 134.7s and 116.3s on the candidates; this is work summed across tests, not gate time.
The observed full-gate reduction against the fresh baseline is 14–111s (2.6–20.4%). This is not evidence of a stable 20% gain: candidate timing varied substantially, including gaps between worker groups and changes in untouched file timings. Both samples are reported rather than selecting the fastest. Human/code-owner review is still required.
With the clipping fix included, the fresh gate was 35s (6.4%) shorter than the same baseline; the slowest browser step was 42.1s shorter. Summed journey execution fell from 2654.5s to 2393.3s (9.8%), not wall time. On the critical WebKit shard, initial-position fell from 107.2s to 65.4s summed execution; image-scroll became the slowest file at 77.2s. This additional sample supports a gain for the combined PR, not a speed claim for the clipping fix itself or a stable percentage.